Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility to define cluster name to configuration #26

Closed
wants to merge 1 commit into from

Conversation

zetaab
Copy link

@zetaab zetaab commented Jun 11, 2019

No description provided.

@Issif
Copy link
Member

Issif commented Jun 11, 2019

Hi,

Thanks for your PR, I reviewed it quickly, all seems OK.

Anyhow, you gave me an idea, specifying a list of key:values for adding anything we want as fields. What do you think about? It's only a generic pattern of you have done.

@zetaab
Copy link
Author

zetaab commented Jun 11, 2019

@Issif the problem what I see is that its little bit difficult to support yaml file and ENV variables in that key:value thing. Okay it works if we just define like ExtraLabels string. Then it could be something like

		extraLabels := strings.Split(config.ExtraLabels, ",")
		for _, label := range extraLabels {
			splitted := strings.Split(label, ":")
			if len(splitted) == 2 {
				field.Title = splitted[0]
				field.Value = splitted[1]
				field.Short = true
				fields = append(fields, field)
			}
		}

This works with env variables quite nice, if you just define export EXTRALABELS=foo:bar,heh:hah. However, it looks little bit stupid in yaml file?

@Issif
Copy link
Member

Issif commented Jun 11, 2019

I agree it seems weird in YAML to list elements in a single line like with envvars. I need to check how viper package handles that cases, in classic YAML we can provide a list of key:value like that normally :

parameters:
  firstKey: value1
  secondKey: value2

@Issif
Copy link
Member

Issif commented Jun 12, 2019

@zetaab

I did it in branch : https://github.com/Issif/falcosidekick/tree/custom_fields

We can now add any key:value we want via .yaml or envvars.

image

I updated the README in the dedicated branch, can you please test on your side?

@zetaab
Copy link
Author

zetaab commented Jun 12, 2019

@Issif for me it looks what I want. And that will help others as well in many usecases. Feel free to push your way forward :)

@zetaab zetaab closed this Jun 12, 2019
@Issif
Copy link
Member

Issif commented Jun 12, 2019

@zetaab

Thanks for your idea and comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants